West Midlands | 26-ITP-May | Alina Sofragiu | Sprint 2 | Book-library - #508
West Midlands | 26-ITP-May | Alina Sofragiu | Sprint 2 | Book-library#508sofragiualina wants to merge 8 commits into
Conversation
sofragiualina
added
📅 Sprint 2
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
|
I've gone through the review and updated my project I fixed the HTML validation issues, improved the input validation, renamed variables to be more descriptive, used textContent instead of innerHTML where appropriate, stored the page count as a number and made other improvements |
sofragiualina
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look solid. I only have a few comments.
| <label for="pages">Pages</label> | ||
| <input type="number" id="pages" class="form-control" min="1" required /> |
There was a problem hiding this comment.
Why not make this input element to accept only positive whole number?
| const pages = Number(pagesInput.value); | ||
|
|
||
| if (title === "" || author === "" || !pages) { |
There was a problem hiding this comment.
What if pagesInput.value is "1.234"?
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" | ||
| onclick="submit()" |
There was a problem hiding this comment.
Why not attach submit() as the callback in the JavaScript code (instead of specifying it in HTML)?
|
|
||
| render(); | ||
|
|
||
| alert(`You've deleted "${deletedTitle}".`); |
There was a problem hiding this comment.
alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.
If time permits, research for approaches that allows the UI to update before displaying the alert dialog. (This is an optional change).
cjyuan
added
Reviewed
Added step attribute to pages input
…dation for book details
Removed the 'step' attribute from the pages input field.
|
applied the suggested changes, thank you |
sofragiualina
added
Needs Review
2 participants
… functionality
Learners, PR Template
Self checklist
Changelist
Fixed the Book Library bugs